-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TYP: misc fixes for numpy types 3 #36100
Conversation
simonjayhawkins
commented
Sep 3, 2020
@@ -5076,7 +5076,7 @@ def _dtype_to_kind(dtype_str: str) -> str: | |||
return kind | |||
|
|||
|
|||
def _get_data_and_dtype_name(data: ArrayLike): | |||
def _get_data_and_dtype_name(data: AnyArrayLikeUnion): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better to ensure we unwrap to avoid Index/Series?
for my edification: i was under the impression that AnyArrayLike vs AnyArrayLikeUnion only mattered when we had more than one of them in a signature; i.e.
def foo(arg: AnyArrayLike) -> AnyArrayLike:
implies the return type matches the arg type, while using AnyArrayLikeUnion
does not imply that. if that's correct, does it matter which we use in cases like this with only one annotation in the signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case we change the type of data in the body. i.e assign to data and hence no preserving type of data.
I've used ..Union to be consistent with FrameOrSeries.
we could change ArrayLike and AnyArrayLike to be unions and ArrayLikeT and AnyArrayLikeT for type variables.
or we could assign to another variable name in the body (simpler, but imo messy and less clear)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im happy to follow your lead on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we could assign to another variable name in the body (simpler, but imo messy and less clear)
I don't think worth going down this path. We will likely need a Union type for variable type annotations anyway, since if we use ArrayLike we would get error: Type variable "pandas._typing.ArrayLike" is unbound [valid-type]
@@ -40,7 +40,9 @@ | |||
# array-like | |||
|
|||
AnyArrayLike = TypeVar("AnyArrayLike", "ExtensionArray", "Index", "Series", np.ndarray) | |||
AnyArrayLikeUnion = Union["ExtensionArray", "Index", "Series", np.ndarray] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment on when you should use the AnyArryLike TypeVar and when the Union (for readers of this code!)
pls rebase |
closing to clear queue. (also need a decision on naming) |